Netlify deploy preview

https://deploy-preview-39137--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a8b36c2

Thanks for the PR. Please take a look at the failing CI checks. Also, please rebase on the latest master (this should help with the failing Argos check).

@michaldudak Thanks! I made some more changes that I think solves the issue well. Should I create some tests for PR?

After my rebase I get this error however, which I find confusing since I have not altered any files in either mui-base or the corresponding component. Do you have any idea how I can solve it?

\GolandProjects\material-ui\packages\mui-base\src\useTabPanel\useTabPanel.test.js
4:51 error Unable to resolve path to module 'test/utils' import/no-unresolved

@ZeeshanTamboli Thanks for the answer! Are you sure that I am supposed to the values inside the Select.d.ts file? Based on the reffered pull request (#36737) in the issue (#38487) I assumed that we wanted to assert our types based on the given Select variant inside of the component. By doing this we can pass the correct type of InputProps to the Select component and avoid the typeError.

Could you provide some additional information on how you would want that implemented inside of the Select component itself?

@PaulKristoffersson Since TablePagination uses SelectProps of the Select component, I think it makes sense to fix it in the Select component types itself. As an example, I think when the Select component has filled variant, hiddenLabel prop should be available:

<Select variant="filled" hiddenLabel /> ---> Type error for hiddenLabel

I assumed that we wanted to assert our types based on the given Select variant inside of the component. By doing this we can pass the correct type of InputProps to the Select component and avoid the typeError.

We want the type to be based on the select variant. I didn't understand what you mean by the InputProps for TablePagination SelectProps.

Can you confirm the fix by adding TypeScript tests?

@ZeeshanTamboli Hey, I just came back to this issue after having worked on some other ones, sorry for the delay. In response to:

"I think it makes sense to fix it in the Select component types itself. As an example, I think when the Select component has filled variant, hiddenLabel prop should be available:"

As far as I can tell Select does not contain the hiddenLabel prop, instead the hiddenLabel prop is contained in one of the variations of the inputProps prop, namely FilledInput.

Do you want me to add hiddenLabel as a prop? Otherwise I can try to add some tests to show how my solution works.

@ZeeshanTamboli Just noticed that the SelectProps prop is deprecated and we slotProps.select instead, so I assume that there is not point on working with this issue anymore?

As far as I can tell Select does not contain the hiddenLabel prop. Do you want me to add hiddenLabel as a prop? Otherwise I can try to add some tests to show how my solution works.

It doesn't have the hiddenLabel prop on its own, but when the variant="filled" is applied, the Select component renders the FilledInput component, which can be seen here. Therefore, the TypeScript definitions for Select should include the props of FilledInput when variant is filled, which includes the hiddenLabel prop. Feel free to add tests.

Just noticed that the SelectProps prop is deprecated and we slotProps.select instead, so I assume that there is not point on working with this issue anymore?

But the slotProps.select has the same type - Partial<SelectProps>. So the fix should be applied for it instead.

@ZeeshanTamboli Thank you for the answer! I moved my changes to slotProps, and instead of focusing on InputProps I now extend the SelectProps with the different props based on the Select variant. I get no type errors with the component in my local playground with my component as such:

<TablePagination size="small" component="div" count={10} page={page} rowsPerPage={10} onPageChange={handlePageChange} slotProps={{ select: { variant: "filled", size: "small", hiddenLabel: true, disableUnderline: true } }} />

Do you have any suggestion on what test I should make to confirm the behaviour?

@ZeeshanTamboli I have added a test now as well. If you have any inputs on it they are very welcome :)

Can you include the test for issue #38487 in the TablePagination.spec.tsx file?

Added the test 👍

The switch from interface to type also breaks for anyone extending it.

interface MyCustomSelectProps extends SelectProps {} no longer works as types can't be extended but have to use an intersection or a union

Also, the variant "standard" broke and is now recognized as a error by typescript:

I started having issues here.

Type '{ autoWidth?: boolean | undefined; children?: ReactNode; classes?: (Partial<SelectClasses> & Partial<FilledInputClasses>) | undefined; ... 309 more ...; hiddenLabel?: boolean | undefined; } | { ...; } | { ...; }' is not assignable to type 'IntrinsicAttributes & { variant?: "outlined" | undefined; } & Omit<SelectProps<Value, "outlined">, "variant">'.
  Type '{ autoWidth?: boolean | undefined; children?: ReactNode; classes?: (Partial<SelectClasses> & Partial<FilledInputClasses>) | undefined; ... 309 more ...; hiddenLabel?: boolean | undefined; }' is not assignable to type '{ variant?: "outlined" | undefined; }'.
    Types of property 'variant' are incompatible.
      Type '"filled"' is not assignable to type '"outlined"'.

Do you have any suggestions on how to resolve this? It is breaking our CI.

Hey everyone! I'm sorry for any trouble this might have caused.

As an immediate workaround, if you were using SelectProps directly, please try using BaseSelectProps instead.

Unfortunately, I don't think that would work for @damisparks case. I'll take a look into that. I'll get back to you as soon as possible.

@damisparks in the meantime, this workaround might help

import {
  Select as MaterialSelect,
  SelectProps as MaterialSelectProps,
  SelectVariants as MaterialSelectVariants,
} from '@mui/material';

type SelectProps<Value, Variant extends MaterialSelectVariants> = { variant?: Variant } & Omit<
  MaterialSelectProps<Value, Variant>,
  'variant'
>;

export default function Select<Value, Variant extends MaterialSelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

Hey everyone! I'm sorry for any trouble this might have caused.

As an immediate workaround, if you were using SelectProps directly, please try using BaseSelectProps instead.

Unfortunately, I don't think that would work for @damisparks case. I'll take a look into that. I'll get back to you as soon as possible.

@DiegoAndai I tried the BaseSelectProps, but it did not work as you suspected. I already tried that before commenting here.

@damisparks in the meantime, this workaround might help

import {
  Select as MaterialSelect,
  SelectProps as MaterialSelectProps,
  SelectVariants as MaterialSelectVariants,
} from '@mui/material';

type SelectProps<Value, Variant extends MaterialSelectVariants> = { variant?: Variant } & Omit<
  MaterialSelectProps<Value, Variant>,
  'variant'
>;

export default function Select<Value, Variant extends MaterialSelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

@DiegoAndai, I appreciate the workaround. It helped to resolve the TS errors.

The final implementation looks like this.

import MuiSelect, {
  type SelectProps as MuiSelectProps,
  type SelectVariants as MuiSelectVariants,
} from "@mui/material/Select";

import copyComponentStaticProps from "../copyComponentStaticProps.js";

// The function `wrapComponent` cannot be used because this component has a
// generic type parameter.

/**
 * A [Material UI](https://mui.com/material-ui) React component for a select
 * input.
 *
 * - [Material UI guide](https://mui.com/material-ui/react-select)
 * - [Material UI API docs](https://mui.com/material-ui/api/select)
 * @param props Props.
 * @returns Select JSX.
 */
function Select<Value, Variant extends MuiSelectVariants>(
  props: {
    variant?: Variant;
  } & Omit<MuiSelectProps<Value, Variant>, "variant">,
) {
  return <MuiSelect<Value, Variant> {...props} />;
}

copyComponentStaticProps(MuiSelect, Select);

export default Select;

I would expect this to work:

import { Select as MaterialSelect, SelectProps, SelectVariants } from '@mui/material';

export function MySelect<Value, Variant extends SelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

But it isn't. Maybe I'm misunderstanding something. Or perhaps it is related to microsoft/TypeScript#10571 / microsoft/TypeScript#23696 / microsoft/TypeScript#26242. If someone figures out why it isn't working, please reach out 😊.

I'll bring this topic to the MUI Core team to gather ideas.

@DiegoAndai, Should I raise an issue for this topic? Otherwise, I am afraid the conversation will get lost under this PR.

@DiegoAndai Even though the code below fits the TS errors.

import MuiSelect, {
  type SelectProps as MuiSelectProps,
  type SelectVariants as MuiSelectVariants,
} from "@mui/material/Select";

function Select<Value, Variant extends MuiSelectVariants>(
  props: {
    variant?: Variant;
  } & Omit<MuiSelectProps<Value, Variant>, "variant">,
) {
  return <MuiSelect<Value, Variant> {...props} />;
}

copyComponentStaticProps(MuiSelect, Select);

It does not address choosing a concrete variant for the second generic argument. At the same time, the story (i.e.) Select on the storybook is supposed to be a component with multiple variants and ensure the props table is accurate when the selected variant changes. We are using react-docgen-typescript to generate docs for component props.

Conceptually, MUI has combined multiple components with different props, HTML elements and styles into one Select component. The allowed props are different depending on the variant prop.

It makes sense that the storybook cannot generate the correct props documentation and controls for the MUI Select because it can only generate one set of props to display statically. Still, this component has more than one set of props. It has one set per variant.

Should I raise an issue for this topic?

@damisparks yes please! Let's open a new issue to continue this discussion. Tag me on it as well. Please add a live reproduction that demonstrates the typescript error. You can also add a live reproduction for the storybook case.

@damisparks Someone else opened a related issue, but I think it's not quite the same, so let's open yours as well.

@DiegoAndai, thanks for the update. I will raise an issue and provide minimal storybook case reproduction.

Hey everyone, we've been discussing this PR's implementation and changes in #41405 as it introduced some regressions. We're thinking of modifying it. If you're interested, please take a look and join the conversation.